Approach makes sense to me. Thank you.

on this implementation, now a theme has the possibility to define only a few variables, with the default values of cssVars being used for the undefined variables. I'm not sure this is a great idea as it could lead to oversights when implementing new themes or updating current ones. I'm not sure it's actually an issue either. As we can easily create a new Theme by extending an existing one anyway. What do you think?

Making all the tokens required in ThemeColors would be my preference. In fact, I'd be interested in taking the refactor further and trying to get it down to 3 layers: grist-base, grist-theme, and grist-custom. My thinking is a theme should always be applied, so we could try always applying the light theme on initial setup (i.e. from attachCssRootVars), which would replace the role that grist-tokens currently serves. Do you see any problems with doing that?

Approach makes sense to me. Thank you.

Great :)

Making all the tokens required in ThemeColors would be my preference

So to be 100% clear, you mean at least this?

• a theme object (like GristLight.ts), should define at its root, all the colors, the paddings, font sizes, z-indexes, etc, as now the "design tokens" are a mix between colors and vars,

But do you also mean this?

• it should define in its legacyVariables prop (current naming in the PR), the 400+ component-specific variables, with stuff mostly targeting the design tokens.

I'd argue if we think those component-specific variables are kinda legacy and we eventually want to remove them, I'd say making them required in a theme object is maybe too much trouble? Since the idea is themes would not want to change those, as cssVars.theme would mostly default to design tokens.

Here are a few thoughts:

• if a theme *has* to implement all designTokens, there would be no need to define any value in cssVars.designTokens, since all actual values would always come from the current theme. The definition of those objects would then mostly help only to easily target css variables from the JS code when writing css. Maybe I could update the code to more reflect that and stop defining actual values in designTokens, but only define css variable keys?
• would it make sense to change how we set up values in a theme? Instead of having props matching css variable names, having them match the designTokens and theme keys. Maybe feels weird to have all the code target the "js props" while a theme targets "css variables"?
• I'd say having some 'BaseTheme' object that lists all the default stuff would be helpful. Like all the padding/fonts/zindex. So that a theme could be easily defined by listing all its colors, that wouldn't be in the BaseTheme, and merging the BaseTheme, for all the stuff that is common.

In fact, I'd be interested in taking the refactor further and trying to get it down to 3 layers: grist-base, grist-theme, and grist-custom. My thinking is a theme should always be applied, so we could try always applying the light theme on initial setup (i.e. from attachCssRootVars), which would replace the role that grist-tokens currently serves.

Yeah I guess this way of doing was more to show the structure, indeed "grist-tokens" is not necessary. I'd argue if *everything* is listed in a theme object, the concept of "grist-base" variables is also debatable: every design token variable is tied to the theme, and then there is the custom css layer that comes on top.

So to be 100% clear, you mean at least this?

a theme object (like GristLight.ts), should define at its root, all the colors, the paddings, font sizes, z-indexes, etc, as now > the "design tokens" are a mix between colors and vars,

Only the variables we'd like to change from theme to theme. So yes to colors, but no to z-indexes.

Padding and font size are more interesting; I can see an argument that making them required would be more of a nuisance, as they won't always change across themes. A compromise might be to make the vars (e.g. fonts, padding, etc.) optional, and the colors required, with the vars being inherited from the base layer.

But do you also mean this?

it should define in its legacyVariables prop (current naming in the PR), the 400+ component-specific variables, with stuff mostly targeting the design tokens.
I'd argue if we think those component-specific variables are kinda legacy and we eventually want to remove them, I'd say making them required in a theme object is maybe too much trouble? Since the idea is themes would not want to change > those, as cssVars.theme would mostly default to design tokens.

No, I agree that it's too much trouble to make those required.

Here are a few thoughts:

• if a theme has to implement all designTokens, there would be no need to define any value in cssVars.designTokens, since all actual values would always come from the current theme. The definition of those objects would then mostly help only to easily target css variables from the JS code when writing css. Maybe I could update the code to more reflect that and stop defining actual values in designTokens, but only define css variable keys?
• would it make sense to change how we set up values in a theme? Instead of having props matching css variable names, having them match the designTokens and theme keys. Maybe feels weird to have all the code target the "js props" while a theme targets "css variables"?
• I'd say having some 'BaseTheme' object that lists all the default stuff would be helpful. Like all the padding/fonts/zindex. So that a theme could be easily defined by listing all its colors, that wouldn't be in the BaseTheme, and merging the BaseTheme, for all the stuff that is common.

I had a similar thing in mind. A base theme defining the default, non-color variables makes a lot of sense to me. The current system is awkward because things live in two places, as you said. If we can narrow that down to one, that'd be great!

Hey @georgegevoian,

Here is an update to move things forward.

Summary

The idea is:

• Actual values are now only in themes, except for css vars that are really theme-agnostic, which means now only the z-index list. All other variables (colors, font families, paddings, font sizes, etc) can now be modified in a theme.

• New tokens and legacyVariables objects list CustomProp instances for all theme tokens. These are here mostly so that the codebase can consume the theme tokens.
  For example, instead of writing this in a component code:

  import {colors, vars, theme} from 'app/client/ui2018/cssVars';
  export const button = styled('button', `
    font-size: ${vars.mediumFontSize};
    color: ${colors.slate};
    background-color: ${theme.controlPrimaryBg};
  `);

  We would now write:

  import {tokens, legacyVariables} from 'app/common/ThemePrefs';
  export const button = styled('button', `
    font-size: ${tokens.mediumFontSize};
    color: ${tokens.slate};
    background-color: ${legacyVariables.controlPrimaryBg};
  `);

  For now, to prevent changing hundreds of files and have an even more difficult diff to read (and also while this is still an ongoing discussion :)), old cssVars colors, vars and theme are still there, but just target the new objects.

• A new Base theme implements the theme tokens that we assume won't change much between actual themes. It mostly lists old vars and theme variables, the grayscale that was in colors, and added generic variables. The theme variables are listed under a legacyVariables property as suggested before.

• Each theme must describe mandatory keys, that are not in the Base theme: specific colors.

• I started some work to add some generic variables to let us implement less things on a specific theme's legacyVariables. You can see around 230 legacy variables now target generic tokens, while 200 others are still implemented in each theme. This gist helps in finding common values mapping between the light and dark themes.

Feedback needed

Variable namings and impact on widget's styling

I made the choice to abstract the css variable names as much as possible. The goal is that we don't have to regularly switch in our heads between the consumed in the codebase name (tokens.textLight), vs the described in the theme name (theme-color-text-light).

So there is some code to map some js object keys to existing css var names.

Unfortunately I only figured out later that theme variables in widget iframes was loaded differently. And from what I understand I can't import common files in the plugin-related code? Which means for now I can't go find the css variable name matching the theme key in the plugin code, like I do in the default attachCssThemeVars.

Any advice on what I could do there?

My intuition would be to actually remove the js object key <> css var name manual mapping and just generate css variable names based on the JS keys. So that tokens.textLight would generate a css variable --grist-textLight automatically. I did a manual mapping and kept the current css names for potential backward compatibility with custom css files and to prevent having to update files here and there directly consuming css variables… But your input would be helpful, from my limited point of view I'd find it simpler if we didn't have to handle this mapping. I actually don't get what is the value in handling ourselves the css variable names instead of relying on our JS namings that we use across most of the app.

TS checkers

I had trouble with the ts-interface-checker related code, because it doesn't handle generics. I ended up just… removing the ThemePrefs-ti file. So I replaced a specific part of the app where some checkers were used (in gristUrls) to make the check manually. And I removed the checker in AppModel. I actually don't understand that much why this checker is needed here, since the themes are checked by typescript at build time and from my understand at this stage the theme can't come from a random runtime value?

I'm not against a little bit of explanation here if what I did is not ok, I assume I missed something :)

Next steps

If what you see seems globally find for you, here what I wanted to continue on:

• fix the theme variables loading for iframes
• actually remove cssVars.ts colors, vars and theme and make the whole codebase target the new tokens variable. I guess changing this in one big commit would be better than leaving 2 ways of consuming design tokens in the codebase
• think a little more about new generic names for the legacyVariables, meaning, continuing the work I started with the 230 vs 200 legacyVariables (see above). This includes both putting out of "legacyVariables" some interesting variables (like input or control related ones), and coming up with new generic variables that legacy variables would then consume. My goal is not to clean this very thoroughly as there are a lot of stuff. But at least cleaning the variables used in multiple places and come up with better namings. I don't see a lot of value in taking hours cleaning all the variables used in only specific places, but I see some in cleaning the variables used more globally.

What do you think?

Thanks a lot.